Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bug 1164205] Refactor search suggest api #2525

Merged
merged 2 commits into from
May 13, 2015
Merged

[bug 1164205] Refactor search suggest api #2525

merged 2 commits into from
May 13, 2015

Conversation

willkg
Copy link
Member

@willkg willkg commented May 13, 2015

This refactors the search suggest API to use more DRF-y things including
built-in DRF-y validation.

In the process of doing this, I changed it so that you can provide the filter
parameters in the querystring OR in the HTTP request body as JSON.

r?

This refactors the search suggest API to use more DRF-y things including
built-in DRF-y validation.

In the process of doing this, I changed it so that you can provide the filter
parameters in the querystring OR in the HTTP request body as JSON.
@willkg
Copy link
Member Author

willkg commented May 13, 2015

This curl stuff helps test:

curl -X GET "http://localhost:8000/api/2/search/suggest/?q=videos&max_documents=3&max_questions=0"

echo ""

curl -X GET http://localhost:8000/api/2/search/suggest/ \
     -H "Content-Type: application/json" \
     -d '
{
   "q": "videos",
   "max_documents": 3,
   "max_questions": 0
}'

echo ""

curl -X GET "http://localhost:8000/api/2/search/suggest/?q=videos" \
     -H "Content-Type: application/json" \
     -d '
{
   "max_documents": 3,
   "max_questions": 0
}'

@willkg
Copy link
Member Author

willkg commented May 13, 2015

The Travis failure is from failing to download or not having simplejson which isn't related to these changes. It's either a spurious error or related to the master branch.

@@ -11,49 +12,75 @@
from kitsune.wiki.models import DocumentMappingType


def positive_integer(value):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need to check if it's an int in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The serializer has it as an IntegerField, so that should do the integer validation. Pretty sure one of the existing tests covers that scenario.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I typed before scrolling

I have no idea if this is the format we want to go with, but it is some
documentation.

Maintaining it will be painful. It's probably better for us to figure
out a way to autogenerate the documentation or somehow move the
documentation to the code. That work is covered in bug #1164247.
@willkg
Copy link
Member Author

willkg commented May 13, 2015

^^^ Add some rough docs for the sumo search suggest api.

data = json.dumps({'q': 'emails'})
# Note: Have to use .generic() because .get() will convert the
# data into querystring params and then it's clownshoes all
# the way down.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not 👠 dance shoes?

@rlr
Copy link
Contributor

rlr commented May 13, 2015

I ran tests. Did some manual tests via browser and curl. My machine didn't catch 🔥.

r+

rlr added a commit that referenced this pull request May 13, 2015
[bug 1164205] Refactor search suggest api
@rlr rlr merged commit 919a862 into mozilla:master May 13, 2015
@willkg willkg deleted the 1164205-json-me branch May 13, 2015 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants